Skip to content

**refactor: migrate QueueEntryDaoImpl from deprecated Hibernate Criteria API to JPA CriteriaBuilder**#101

Open
shubhangiisinghh wants to merge 1 commit intoopenmrs:mainfrom
shubhangiisinghh:feat/optimise-queue-entry-dao
Open

**refactor: migrate QueueEntryDaoImpl from deprecated Hibernate Criteria API to JPA CriteriaBuilder**#101
shubhangiisinghh wants to merge 1 commit intoopenmrs:mainfrom
shubhangiisinghh:feat/optimise-queue-entry-dao

Conversation

@shubhangiisinghh
Copy link

Migrate QueueEntryDaoImpl to JPA CriteriaBuilder

getQueueEntries() and getCountOfQueueEntries() were still using the legacy Hibernate Criteria API, which has been deprecated since Hibernate 5.2. getOverlappingQueueEntries() in the same class already used JPA CriteriaBuilder — this PR brings the other two methods in line with that.
The main changes:

Replaced org.hibernate.Criteria / Restrictions / Order with CriteriaBuilder equivalents
Pulled the shared filter logic into a buildPredicates() helper to avoid duplicating it across both methods
Added a small limitCollection() utility that mirrors the behaviour of the old limitByCollectionProperty() from the parent class — null collection = no filter, empty collection = IS NULL, non-empty = IN (...)

No behaviour changes, all existing tests pass.

…Builder in QueueEntryDaoImpl

getQueueEntries() and getCountOfQueueEntries() were still using the legacy
org.hibernate.Criteria API (deprecated since Hibernate 5.2). Migrated both
to JPA CriteriaBuilder, consistent with getOverlappingQueueEntries() which
already used it. Extracted shared predicate logic into buildPredicates().
@shubhangiisinghh
Copy link
Author

Note: Will link a Jira ticket once I get access to issues.openmrs.org.

This is part of a broader effort to modernise the DAO layer ahead of a potential Hibernate 6 migration. Happy to split further or adjust scope based on feedback.

@RoshanMadival08
Copy link

One suggestion: consider adding a brief Javadoc on buildPredicates() explaining the filter logic for future contributors unfamiliar with the original Criteria API behaviour.

@sanks011
Copy link

Hey @shubhangiisinghh! Great refactor! Migrating away from the legacy Criteria API is definitely the right move. I read through the changes and noticed a subtle behavioral difference regarding how the queue join translates to SQL:

  1. Inner Join vs Left Join: In the original code, c.createAlias("queue", "q") was used. In the legacy Hibernate Criteria API, createAlias defaults to an INNER JOIN. In the new code (line 115), you've used root.join("queue", JoinType.LEFT).
    If a QueueEntry can ever have a null queue, this implicitly changes the result set (the old code would filter them out, the new code will include them). Even if queue is mandatory and cannot be null, an INNER JOIN is strictly more performant and faithfully matches the legacy generated SQL.

  2. Avoiding Double Joins: Currently, the code uses root.get("queue") on line 113 for the .in() clause, and then explicitly creates root.join("queue", ...) on line 115. Depending on the Hibernate version, mixing implicit path navigation (get) with an explicit join on the same entity can cause JPA to generate two separate joins in the underlying SQL (one cross/inner join, one left join).

Since the old code unconditionally created the alias anyway, a safer and more exact 1:1 translation would be to unconditionally create an INNER JOIN up front, and reuse it for all the queue checks to prevent duplicate joins:

Join<QueueEntry, Queue> queueJoin = root.join("queue", JoinType.INNER);

// Use the explicit join instead of root.get("queue")
limitCollection(predicates, queueJoin, searchCriteria.getQueues());

if (searchCriteria.getLocations() != null || searchCriteria.getServices() != null) {
    limitCollection(predicates, queueJoin.get("location"), searchCriteria.getLocations());
    limitCollection(predicates, queueJoin.get("service"), searchCriteria.getServices());
}

Other than that, the limitCollection mapping for the empty list vs null checks is spot on. Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants